-
Notifications
You must be signed in to change notification settings - Fork 3.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Doc] Introduction to module serialization #4564
Conversation
Would be great if we can get more readers to help review |
678e2c5
to
80e2414
Compare
It would be great if we can add a figure explaining the storage relation and data structure. |
Will do. |
@tqchen need to double check to make sure I understand correctly. The doc describes:
binary_blob_size
binary_blob_type_key
binary_blob_logic
...
_import_tree
_import_tree_logic <- No need to have figure, as this simple table could describe cleanly.
|
sounds good, would love to see others' thoughts, @zhiics @yzhliu @ajtulloch @ZihengJiang |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just some comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few high level comments beside the attached grammatical errors and typos.
- A diagram would be more helpful for ppl to understand. I think the one Tianqi provided in the RFC could be borrowed
- A little more details about export library can be explained, such as how user provided compilation flags/options can be taken
Thanks the comment and suggestion of @zhiics @yzhliu @yangjunpro I will address the comment in the following days. |
Have addressed the comments. I also add some words of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@FrozenGene would be great to have a few more ppl read through the document because I still see a number of typos and broken sentences.
@yangjunpro @yzhliu please do another around when you have cycles.
ping @zhiics @yzhliu @FrozenGene @yangjunpro please followup on this thread :) |
LGTM now. I will wait till tomorrow to see if there are any more comments. |
Thanks everyone. This is now merged. |
Follow up of #4532, this doc for introducing module serialization and implementation details how we achieve it.
@tqchen